bpo-31955: Fixed incorrect string type check - use isinstance(value, basestring)… #4316

PavelStishenko · 2017-11-07T13:33:11Z

Fixed incorrect string type check. Previously if unicode string was passed as executable value it was considered not as string, but as list.

https://bugs.python.org/issue31955

Previous PR on this issue (#4296) was mistaken - for Python3 str is equivalent to basestring.

… instead of isinstance(value, str)

the-knights-who-say-ni · 2017-11-07T13:33:13Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

PavelStishenko · 2017-11-07T13:56:56Z

This PR is instead of PR #4296 . I agree, that this fix is necessary only for Python 2.

vstinner · 2017-11-07T14:34:46Z

Misc/NEWS.d/next/Library/2017-11-07-19-12-25.bpo-31955.1DWu-S.rst

+distutils.CCompiler.set_executables() method. Previously if unicode string
+was passed as executable value it was considered not as string, but as list.
+It caused TypeError: "coercing to Unicode: need string or buffer, list
+found" at unixccompiler.py, line 122, in _compile().


NEWS entries are read by end users who don't care much to details. I suggest to be much more concise. For example:

Fix CCompiler.set_executable() of distutils to handle properly Unicode strings.

bedevere-bot · 2017-11-07T14:34:54Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

merwok · 2017-11-07T16:52:25Z

Do you think you could add tests for set_executables in Lib/distutils/tests/test_ccompiler.py ?

vstinner · 2017-11-08T10:01:27Z

Lib/distutils/tests/test_ccompiler.py

@@ -24,6 +24,28 @@ def library_option(self, lib):

 class CCompilerTestCase(support.EnvironGuard, unittest.TestCase):

+    def test_set_executables_unicode(self):
+        class MyCCompiler(CCompiler):
+            executables = {'compiler':'', 'compiler_cxx':'', 'linker':''}


PEP 8: {'compiler': '', 'compiler_cxx': '', 'linker': ''}

vstinner · 2017-11-08T10:01:57Z

Lib/distutils/tests/test_ccompiler.py

@@ -24,6 +24,28 @@ def library_option(self, lib):

 class CCompilerTestCase(support.EnvironGuard, unittest.TestCase):

+    def test_set_executables_unicode(self):


Please rename to test_set_executable(), since you test 3 types, not only Unicode.

vstinner · 2017-11-08T10:02:33Z

Lib/distutils/tests/test_ccompiler.py

+        compiler = MyCCompiler()
+
+        # set executable as list
+        compiler.set_executables(compiler = ['/usr/bin/env', 'OMPI_MPICC=clang', '/usr/bin/mpicc.openmpi'])


PEP 8: no space around = when passing keyword argument: set_executables(compiler=[...])

vstinner · 2017-11-08T10:02:53Z

Lib/distutils/tests/test_ccompiler.py

+
+        # set executable as list
+        compiler.set_executables(compiler = ['/usr/bin/env', 'OMPI_MPICC=clang', '/usr/bin/mpicc.openmpi'])
+        print(compiler.executables)


Remove debug prints please.

vstinner · 2017-11-08T10:03:37Z

Lib/distutils/tests/test_ccompiler.py

+        compiler.set_executables(compiler = ['/usr/bin/env', 'OMPI_MPICC=clang', '/usr/bin/mpicc.openmpi'])
+        print(compiler.executables)
+        self.assertEqual(len(compiler.executables['compiler']) == 3)
+        self.assertEqual(len(compiler.executables['compiler'][2]) == '/usr/bin/mpicc.openmpi')


Hum, I dislike testing only one value. Please test:

self.assertEqual(compiler.executables['compiler'], ...) # fill the dots ;-)

vstinner · 2017-11-08T10:04:05Z

Lib/distutils/tests/test_ccompiler.py

+        # set executable as unicode string
+        compiler.set_executables(linker = u'/usr/bin/env OMPI_MPICXX=clang++ /usr/bin/mpicxx.openmpi')
+        self.assertEqual(len(compiler.executables['linker']) == 3)
+        self.assertEqual(len(compiler.executables['linker'][2]) == '/usr/bin/mpicxx.openmpi')


Use a Unicode string here as well.

PavelStishenko · 2017-11-08T12:04:49Z

I have made the requested changes; please review again.

bedevere-bot · 2017-11-08T12:04:51Z

Thanks for making the requested changes!

@Haypo: please review the changes made to this pull request.

eamanu

Good

PavelStishenko · 2017-11-08T13:25:22Z

I have made the requested changes; please review again.

bedevere-bot · 2017-11-08T13:25:24Z

Thanks for making the requested changes!

@Haypo: please review the changes made to this pull request.

vstinner · 2017-11-08T13:38:23Z

Lib/distutils/tests/test_ccompiler.py

+
+        # set executable as list
+        compiler.set_executables(compiler=['env', 'OMPI_MPICC=clang', 'mpicc'])
+        self.assertEqual(len(compiler.compiler), 3)


Checking the length became redundant with the following check and so should be removed.

vstinner · 2017-11-08T13:39:24Z

Lib/distutils/tests/test_ccompiler.py

+                                                 'mpicxx'])
+
+        # set executable as unicode string
+        compiler.set_executables(linker=u'env OMPI_MPICXX=clang++ mpicxx')


Can you please use a different string, like "env OMPI_MPICXX=clang++ linker" or whatever different than the previous test.

vstinner · 2017-11-08T13:39:50Z

The change now looks to me, but I have a last minor request, sorry!

PavelStishenko · 2017-11-08T14:32:39Z

I have made the requested changes; please review again.

bedevere-bot · 2017-11-08T14:32:41Z

Thanks for making the requested changes!

@Haypo: please review the changes made to this pull request.

vstinner · 2017-11-08T15:37:51Z

Hum, @Mazay0, it seems like you didn't sign the CLA. What is your bugs.python.org login? If you are Dee Mee, please fill your GitHub login in your bugs.python.org profile: click on "Your Details".
https://bugs.python.org/user27322

PavelStishenko · 2017-11-08T15:47:19Z

If you are Dee Mee, please fill your GitHub login in your bugs.python.org profile: click on "Your Details".

Done!

vstinner · 2017-11-08T15:48:03Z

Done!

I confirm: after removing the "CLA not signed" label, the bot added the green "CLA signed" label.

vstinner · 2017-11-08T15:49:50Z

Thanks @Mazay0 and congratulations for your first PR! 🎉

eamanu · 2017-11-08T15:57:21Z

@Mazay0 congratulation!

PavelStishenko added 2 commits November 7, 2017 15:26

fixed incorrect string type check - use isinstance(value, basestring)…

034bffd

… instead of isinstance(value, str)

added News entry

736bb22

the-knights-who-say-ni added the CLA not signed label Nov 7, 2017

bedevere-bot added the awaiting review label Nov 7, 2017

PavelStishenko mentioned this pull request Nov 7, 2017

bpo-31955: Correct distinguish of list and string values in set_executable() #4296

Closed

vstinner requested changes Nov 7, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Nov 7, 2017

PavelStishenko added 3 commits November 8, 2017 10:39

fixed News entry, added test for set_executables

57d5b70

fixed test

6ef43e2

fixed test 2

97896f3

vstinner reviewed Nov 8, 2017

View reviewed changes

PavelStishenko added 6 commits November 8, 2017 16:53

fixed asserts

fc14e16

pep8 fixes

e7e3860

fixed assert with Unicode string

0127669

fixed asserts again

73b958e

fixed assert 3

4f0f53c

assert complete list values

831f0d6

bedevere-bot added awaiting change review and removed awaiting changes labels Nov 8, 2017

eamanu approved these changes Nov 8, 2017

View reviewed changes

vstinner reviewed Nov 8, 2017

View reviewed changes

PavelStishenko added 2 commits November 8, 2017 19:55

removed excessive asserts

2f409c4

changed an assert to use different string

45f608b

vstinner removed the CLA not signed label Nov 8, 2017

the-knights-who-say-ni added the CLA not signed label Nov 8, 2017

vstinner removed the CLA not signed label Nov 8, 2017

the-knights-who-say-ni added the CLA signed label Nov 8, 2017

vstinner approved these changes Nov 8, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Nov 8, 2017

vstinner merged commit 8494829 into python:2.7 Nov 8, 2017

bedevere-bot removed the awaiting merge label Nov 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-31955: Fixed incorrect string type check - use isinstance(value, basestring)… #4316

bpo-31955: Fixed incorrect string type check - use isinstance(value, basestring)… #4316

PavelStishenko commented Nov 7, 2017

the-knights-who-say-ni commented Nov 7, 2017

PavelStishenko commented Nov 7, 2017

vstinner Nov 7, 2017

bedevere-bot commented Nov 7, 2017

merwok commented Nov 7, 2017

vstinner Nov 8, 2017

vstinner Nov 8, 2017

vstinner Nov 8, 2017

vstinner Nov 8, 2017

vstinner Nov 8, 2017

vstinner Nov 8, 2017

PavelStishenko commented Nov 8, 2017

bedevere-bot commented Nov 8, 2017

eamanu left a comment

PavelStishenko commented Nov 8, 2017

bedevere-bot commented Nov 8, 2017

vstinner Nov 8, 2017

vstinner Nov 8, 2017

vstinner commented Nov 8, 2017

PavelStishenko commented Nov 8, 2017

bedevere-bot commented Nov 8, 2017

vstinner commented Nov 8, 2017

PavelStishenko commented Nov 8, 2017

vstinner commented Nov 8, 2017

vstinner commented Nov 8, 2017

eamanu commented Nov 8, 2017

		@@ -24,6 +24,28 @@ def library_option(self, lib):

		class CCompilerTestCase(support.EnvironGuard, unittest.TestCase):

		def test_set_executables_unicode(self):

bpo-31955: Fixed incorrect string type check - use isinstance(value, basestring)… #4316

bpo-31955: Fixed incorrect string type check - use isinstance(value, basestring)… #4316

Conversation

PavelStishenko commented Nov 7, 2017

the-knights-who-say-ni commented Nov 7, 2017

PavelStishenko commented Nov 7, 2017

Choose a reason for hiding this comment

bedevere-bot commented Nov 7, 2017

merwok commented Nov 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PavelStishenko commented Nov 8, 2017

bedevere-bot commented Nov 8, 2017

eamanu left a comment

Choose a reason for hiding this comment

PavelStishenko commented Nov 8, 2017

bedevere-bot commented Nov 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner commented Nov 8, 2017

PavelStishenko commented Nov 8, 2017

bedevere-bot commented Nov 8, 2017

vstinner commented Nov 8, 2017

PavelStishenko commented Nov 8, 2017

vstinner commented Nov 8, 2017

vstinner commented Nov 8, 2017

eamanu commented Nov 8, 2017